Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change method to work with explicit nulls #5802

Merged
merged 3 commits into from
Nov 3, 2023

Conversation

rochala
Copy link
Collaborator

@rochala rochala commented Oct 31, 2023

Change required to work with -Yexplicit-nulls according to scala/scala3#18775
This is a blocker for adding -Yexpliict-nulls in the presentation compiler module that will guarantee soundness. scala/scala3#18776

This also may require a minor release.

None
}
} yield value
Option(obj).flatMap { data =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain a bit more why this part was problematic? It seems the same code but without the for comp

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import scala.language.unsafeNulls has some limitations and this is one of it. I think the details can be found here. scala/scala3#18775 (comment)

The issue with for comprehension is that after desugar it runs the yield function in another map so:

for {
  x <- Option(test.fromJson[T]())
} yield (x)

gets desugared into

Option.apply(test.fromJson[T]()).map(x => x)

Thus, according to rules and what was described in dotty issue, as last method is not a direct call to Java method but rather an anonymous function x => x which is basically new symbol, thus the type checking won't be run another time. My changes ensure that Java method is run at the end, thus works with the rules of unsafe nulls.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also add explicit nulls here so that we don't have a regression in the future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking on how to do this, but basically it is not so trivial especially as we cross compile it to both 2 and 3, and what is more we use version 2.13 as a "dependency from source" in dotty. Explicit nulls wasn't present in Scala 2 afaik

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do compile it under Scala 3 though, wouldn't it be enough to add it when the version is Scala 3?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but then we have to add the import scala.language.unsafeNull to every file to compile under Scala 3. And this flag is not present in scala 2. There is also another option to add .nn to every place where explicit nulls is applied but it is also not present for Scala 2.

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tgodzik tgodzik merged commit 84e8151 into scalameta:main Nov 3, 2023
22 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants